[jsweep] Clean validate_lockdown_requirements.cjs#39498
Conversation
- Remove unused ERR_VALIDATION import - Extract failWithError helper to eliminate repeated setOutput+setFailed+throw pattern (DRY) - Use template literals with \n for multi-line error messages instead of string concatenation - Add 6 new tests: setOutput side-effects, multiple tokens, error message newline formatting Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #39498 does not have the 'implementation' label (has_implementation_label=false) and has 0 new lines of code in business logic directories (requires_adr_by_default_volume=false, default_business_additions=0, threshold=100). |
There was a problem hiding this comment.
Pull request overview
This PR performs a small cleanup/refactor of the actions/setup/js/validate_lockdown_requirements.cjs runtime validator (used from github-script), and strengthens its Vitest coverage to ensure behavior and error formatting remain correct.
Changes:
- Removed an unused
ERR_VALIDATIONimport fromvalidate_lockdown_requirements.cjs. - Extracted a
failWithError(message)helper to de-duplicate the repeated failure pattern (setOutput+setFailed+throw). - Added additional tests to validate
setOutputside effects, multi-token configurations, and newline formatting in error messages.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/validate_lockdown_requirements.cjs | Removes unused import, factors repeated failure logic into a helper, and switches error message construction to newline-containing strings. |
| actions/setup/js/validate_lockdown_requirements.test.cjs | Expands coverage to verify no success-path setOutput, multi-token success behavior, and newline presence in error messages. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture and /tdd — approving with two non-blocking suggestions.
📋 Key Themes & Highlights
Key Themes
- Template literal chaining: The
\\n→\nsemantic fix is real and correct, but the strings are still built by concatenating individual template literals. A single template literal (or a true multiline one) would complete the job. .toThrow()consistency: The three new formatting tests use bare.toThrow()while the existing suite already passes a message substring to.toThrow(...). Small gap worth closing.
Positive Highlights
- ✅
@returns {never}JSDoc onfailWithErroris precise — TypeScript will use this for control-flow narrowing. - ✅
failWithErrorDRY extraction eliminates three identical three-liner blocks cleanly. - ✅ The
\\n→\nmigration is a genuine semantic bug fix (old code emitted literal backslash-n text), and the newerror message formattingdescribe block provides direct proof. - ✅ Success-path
setOutputside-effect tests (expect(mockCore.setOutput).not.toHaveBeenCalled()) are excellent regression guards. - ✅ Dead
ERR_VALIDATIONimport removed, eliminating an unnecessary coupling toerror_codes.cjs.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 217.6 AIC · ⌖ 14 AIC · ⊞ 29.4K
|
|
||
| expect(() => { | ||
| validateLockdownRequirements(mockCore); | ||
| }).toThrow(); |
There was a problem hiding this comment.
[/tdd] .toThrow() without an argument does not verify which error is thrown — inconsistent with the existing suite (line 301 uses .toThrow("not compiled with strict mode")).
💡 Match the specificity of existing tests
The pre-existing test block uses:
}).toThrow("not compiled with strict mode");Applying the same pattern here makes all three new formatting tests consistent and catches regressions in the thrown message, not just the fact that something was thrown:
// lockdown test
}).toThrow("no custom GitHub token is configured");
// strict mode test
}).toThrow("not compiled with strict mode");
// pull_request_target test
}).toThrow("pwn request");
There was a problem hiding this comment.
Review
Clean, correct refactor. No blocking issues.
What was examined
failWithError extraction — Correct DRY helper. core.setFailed does not throw in the Actions runtime, so the subsequent throw new Error(message) always executes. The @returns {never} JSDoc annotation is valid and aids TypeScript control-flow analysis under // @ts-check``.
\\n → \n behavioral fix — Old code used "text\\\\n" in regular string literals, producing literal backslash-n in the string value. New code uses `text\n` in template literals, producing real newline characters. @actions/core properly URL-encodes \n → %0A in ::error:: annotations. This is a genuine bug fix that improves log readability.
New tests — The three added describe blocks (setOutput side effects, multiple tokens, error message formatting) validate concrete behaviors: no false-positive output on success paths, correct || token logic, and the \n fix round-trip. Scope is appropriate for unit tests.
🔎 Code quality review by PR Code Quality Reviewer · 340.5 AIC · ⌖ 13.4 AIC · ⊞ 17.3K
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `Lockdown mode is enabled (lockdown: true) but no custom GitHub token is configured.\n` + |
There was a problem hiding this comment.
@copilot refactor to template file and renderTemplate
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. I moved the lockdown/strict/pull_request_target error text into a dedicated template module and render it via renderTemplate.
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `This workflow is running on a public repository but was not compiled with strict mode.\n` + |
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. The strict-mode public-repo failure message now comes from the new template module and is rendered through renderTemplate.
| core.setFailed(errorMessage); | ||
| throw new Error(errorMessage); | ||
| failWithError( | ||
| `This workflow is triggered by the pull_request_target event on a public repository.\n` + |
There was a problem hiding this comment.
Addressed in commit 6c9ce2b. The pull_request_target public-repo failure message was moved to the template module and rendered through renderTemplate.
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (6 tests analyzed)
Go: 0; JavaScript: 6 (
|
There was a problem hiding this comment.
❌ Test Quality Sentinel: 60/100. 50% of new tests are classified as low-value implementation tests, exceeding the 30% threshold. The three error message formatting tests assert on \n presence and exact message wording (internal formatting) rather than behavioral contracts. Please review the flagged tests in the comment above and remove the newline assertions — the content assertions are valuable and should be kept.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Ran the pr-finisher flow and applied the remaining in-scope test feedback in |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…-628d174c9a4092dd
What
Centralise the three lockdown-validation error messages in
validate_lockdown_requirements.cjsinto a dedicated template module, and tighten the accompanying tests.Why
The three error-message strings were constructed inline as raw string concatenations, making them hard to read, maintain, or test independently. Extracting them into named templates:
Additionally, the repeated
setOutput→setFailed→throwpattern appeared three times in the implementation; deduplicating it into afailWithErrorhelper reduces the blast-radius of any future changes to that exit sequence.How
1. New template module —
validate_lockdown_requirements_templates.cjs(added)Exports one render function per error scenario:
renderLockdownTokenErrorMessage()renderPublicStrictModeErrorMessage()--strictrenderPullRequestTargetErrorMessage()pull_request_targettrigger on a public repoEach function calls
renderTemplate(TEMPLATE, TEMPLATE_CONTEXT)frommessages_core.cjs. The sharedTEMPLATE_CONTEXTholdsauth_docs_url,security_docs_url, andstrict_compile_command, so doc URLs and the compile command are defined in exactly one place.2. Implementation cleanup —
validate_lockdown_requirements.cjs(modified)ERR_VALIDATIONimport fromerror_codes.cjs.failWithError(message)helper that performssetOutput("lockdown_check_failed", "true")+setFailed(message)+throw, eliminating three identical call-site copies.requireto the top of the file (style).3. Test improvements —
validate_lockdown_requirements.test.cjs(modified)setOutputside-effects on successful-validation paths (ensureslockdown_check_failedis not set on the happy path).pull_request_targetassertion that duplicated coverage already provided by more focused cases.Files changed
actions/setup/js/validate_lockdown_requirements_templates.cjsactions/setup/js/validate_lockdown_requirements.cjsactions/setup/js/validate_lockdown_requirements.test.cjsNotes
renderTemplateare semantically identical to the previously inlined strings; only the construction mechanism differs.52122d075(narrow POC scope to PR outcomes and safe-output outcomes only), which is unrelated to the lockdown refactor and will be absorbed by the merge.